Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidying __init__ #261

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Tidying __init__ #261

wants to merge 23 commits into from

Conversation

pepoluan
Copy link
Collaborator

@pepoluan pepoluan commented Mar 5, 2021

Depends on #256 , #259 , and #260 ; please merge them first. All deps have been merged. We can go ahead with this one.

What do these changes do?

The __init__ method is very unwieldy. This PR makes it simpler by extracting some logic into properties.

This led to several improvements:

  • event_handler can be replaced "on-the-fly"
  • tls_context can be replaced "on-the-fly" and will automagically change require_tls accordingly
  • User can selectively overload logic in the properties; no longer have to copy > 100 lines of init code just to override one behavior 😉

Are there changes in behavior for the user?

None.

Users have to be aware though that some properties are more 'noisy' in logging.

Related issue number

Inspired partially by #253

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
    • Windows 10 (via PyCharm tox runner)
    • Windows 10 (via PSCore 7.1.2)
    • Windows 10 (via Cygwin)
    • Ubuntu 18.04 on WSL 1.0
    • FreeBSD 12.2 on VBox
    • OpenSUSE Leap 15.2 on VBox
  • Documentation reflects the changes
  • Add a news fragment into the NEWS.rst file

@pepoluan pepoluan added the tech-debt Things that needs to be tidied up to avoid being bitten in the future... label Mar 5, 2021
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #261 (4bd08bb) into master (4bd08bb) will not change coverage.
The diff coverage is n/a.

❗ Current head 4bd08bb differs from pull request most recent head 5f3557c. Consider uploading reports for the commit 5f3557c to get more accurate results

@@            Coverage Diff            @@
##            master      #261   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1706      1706           
  Branches       310       310           
=========================================
  Hits          1706      1706           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pepoluan
Copy link
Collaborator Author

pepoluan commented Mar 5, 2021

Now that I think of it ... this should be merged ONLY after #260 has been merged.

That way, I can test for the behavior of the library when tls_context is being replaced totally. (Current tests are replacing to/from None, and replacing with a new SSLContext object but using the same cert-key pair.)

@pepoluan pepoluan added this to the 1.5 milestone Mar 5, 2021
@pepoluan pepoluan self-assigned this Mar 5, 2021
@pepoluan pepoluan force-pushed the tidy-init branch 4 times, most recently from 94b24db to 6eaef4e Compare March 8, 2021 14:22
@pepoluan pepoluan force-pushed the tidy-init branch 2 times, most recently from e7eff56 to f146e6b Compare March 24, 2021 03:54
@waynew
Copy link
Collaborator

waynew commented Mar 24, 2021

Nice. I'm digging at least the idea :)

Copy link
Collaborator Author

@pepoluan pepoluan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments for possibly interesting decisions

Comment on lines +324 to +346
# Exposed states
session: Optional[Session] = None
envelope: Optional[Envelope] = None

# Protected states
_loop: asyncio.AbstractEventLoop = None
_event_handler: Any = None
_smtp_methods: Dict[str, SmtpMethodType] = {}
_handle_hooks: Dict[str, Callable] = None
_ehlo_hook_ver: Optional[str] = None
_handler_coroutine: Optional[asyncio.Task] = None
_timeout_handle: Optional[asyncio.TimerHandle] = None

_tls_context: Optional[Union[ssl.SSLContext, _Missing]] = MISSING
_req_starttls: bool = False
_tls_handshake_okay: bool = True
_tls_protocol: Optional[sslproto.SSLProtocol] = None
_original_transport: Optional[asyncio.BaseTransport] = None

_auth_mechs: Dict[str, _AuthMechAttr] = {}
_auth_excludes: Optional[Iterable[str]] = None
_authenticator: Optional[AuthenticatorType] = None
_auth_callback: Optional[AuthCallbackType] = None
Copy link
Collaborator Author

@pepoluan pepoluan Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting all these states greatly simplifies the innards of __init__, making it much more readable and thus more maintainable.

Comment on lines +432 to +435
@property
def loop(self) -> asyncio.AbstractEventLoop:
return self._loop
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.loop is now a getter-only property to prevent accidental replacement.

Comment on lines +96 to +97
env = dict(os.environ)
env["PYTHON_EXE"] = str(sys.executable)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also record the Python executable in the env var dump

Comment on lines +146 to +155
def docs_clean(verbose=False):
"""Cleanup build/ to force sphinx-build to rebuild"""
buildpath = Path("build")
if not buildpath.exists():
print("Docs already cleaned.")
return
print("Removing build/ ...", end="")
deldir(buildpath, verbose)
if verbose:
print(flush=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick housekeeping to regen docs

Comment on lines +533 to +535
if not self._auth_mechs:
self._auth_mechs = {}
Copy link
Collaborator Author

@pepoluan pepoluan Mar 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is important that we assign a new dict to this attrib, to prevent 'contamination' between SMTP class instantiations. (Which apparently happens for every new connection to the listening port -- something I just figured out after all these months 😅)

Comment on lines -439 to -443
self._smtp_methods: Dict[str, Any] = {
m.replace("smtp_", ""): getattr(self, m)
for m in dir(self)
if m.startswith("smtp_")
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving method scanning outside of init speeds up class instantiation.

Comment on lines -395 to -404
# Get hooks & methods to significantly speedup getattr's
self._auth_methods: Dict[str, _AuthMechAttr] = {
getattr(
mfunc, "__auth_mechanism_name__",
mname.replace("auth_", "").replace("__", "-")
): _AuthMechAttr(mfunc, obj is self)
for obj in (self, handler)
for mname, mfunc in inspect.getmembers(obj)
if mname.startswith("auth_")
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving method scanning outside of init, especially for a scanning as complex as this, speeds up SMTP instantiation.

@pepoluan
Copy link
Collaborator Author

As soon as test passes on my testing systems, I'm undrafting this and let this marinate.

I might not be able to do code changes for several days ahead, due to me transitioning from "being employed" to "being a free agent" and needing to acquire a new laptop. I reckon up to a week of radio silence due to the Easter holidays. Hopefully not as long as that, depending on how soon the store can fulfill my order.

@pepoluan pepoluan marked this pull request as ready for review March 29, 2021 06:20
@pepoluan
Copy link
Collaborator Author

Nice. I'm digging at least the idea :)

A side effect is that SMTP instatiation is now faster 😉

@waynew
Copy link
Collaborator

waynew commented Nov 25, 2022

@pepoluan I'm going to plan on taking a closer look at this one this week, too. Looks like there are a couple of conflicts, if you're able to tackle those.

Now the common *Controller classes (excluding Base*Controller classes)
are all based on mixins.
Too many pragmas to add.
"Live properties" means properties that, when changed, will immediately
be effective.

A side benefit is that we can move validation/parsing logic out of the
oversized __init__.

The following attributes have been converted:

* `event_handler` -- now on-the-fly event handler replacement is
  possible. Automatically refreshes methods_auth.
* `tls_context` -- now on-the-fly TLS Context replacement (or
  disablement) is possible.
* `require_starttls` -- now automatically adjusts to whether tls_context
  is set or not. Setting this property only sets the "backing" bool
  value.
* `methods_auth` -- read-only property containing authentication
  methods, automatically populated from handler.
* Many internal states moved out of __init__
* Type hint for _smtp_methods now more specific:
  * Define type alias "SmtpMethodType"
  * Define Protocol "HasSMTPAttribs", used by SmtpMethod
* Access to _smtp_methods now via property
  Slightly slower, but this makes __init__ tidier
* Simplify Call Limit mechanism
  * Now uses only one attrib: _call_limit
* Change loop into "read only" property
@pepoluan
Copy link
Collaborator Author

@pepoluan I'm going to plan on taking a closer look at this one this week, too. Looks like there are a couple of conflicts, if you're able to tackle those.

Fixed the conflicts, rebase, and let's see...

@pepoluan
Copy link
Collaborator Author

Oh gosh it blew up! LOL

@pepoluan
Copy link
Collaborator Author

pepoluan commented Jan 17, 2023

Heh seriously at this point I'll just close this PR, but grab the ideas and make a new PR based on the latest master.

There are just too many incremental changes since this PR was created, the proposed changes in this PR makes things go awry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Things that needs to be tidied up to avoid being bitten in the future...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants